Skip to content

perf - combine another astpipeline filter step #12064

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Feb 17, 2025
Merged

perf - combine another astpipeline filter step #12064

merged 2 commits into from
Feb 17, 2025
+330 −22

Conversation

@cscheid
Copy link
Collaborator

@cscheid cscheid commented Feb 11, 2025

(I'm away from the computer I've been running perf tests until the end of the week, but I wanted to run this against our test suite.)

@cscheid
Copy link
Collaborator Author

cscheid commented Feb 17, 2025 •
edited
Loading

The bugfixes also made the code (ever so slightly) faster than the previous one. This is only a 2/775 ~= 0.25% improvement in the Pandoc runtime:

{'outcome': 'max_trials_reached', 'is_significant': False, 'winner': None, 'p_value': np.float64(1.0935405107865948e-05), 'total_samples': 10000, 'alpha_spent': 0, 'statistics': {'counts': {'A': 9411, 'B': 589}, 'means': {'A': 0.7749024699894835, 'B': 0.7731035128109321}, 'variances': {'A': 9.525796573051082e-05, 'B': 9.094457957514734e-05}}}

Statistical Summary:
===================
Variant     Count         Mean      Std Dev
-------- -------- ------------ ------------
A            9411     0.774902     0.009760
B             589     0.773104     0.009536

As you can see, the final p-value is quite small, but because we're doing online testing, the result isn't actually statistically significant (because of multiple comparison corrections). I still feel confident merging it because this summary is consistent from run to run, and removing this step might allow further optimizations later.

@cscheid cscheid force-pushed the perf/astpipeline-2025-02-11 branch from 38af332 to 5aaa6e0 Compare February 17, 2025 23:11
@cscheid cscheid merged commit 0ffc563 into main Feb 17, 2025
5 checks passed
@cscheid cscheid deleted the perf/astpipeline-2025-02-11 branch February 17, 2025 23:13
cderv added a commit that referenced this pull request Apr 7, 2025
@cderv
refactor - parsehtml.lua has been combined in `astpipeline_process_…
0ee649f 
…tables()`

This has been done in #12064

fixup - remove parsehtml.lua
@cderv cderv mentioned this pull request Apr 7, 2025
Merged
cderv added a commit that referenced this pull request Apr 7, 2025
@cderv
refactor - parsehtml.lua has been combined in `astpipeline_process_…
bc1d928 
…tables()`

This has been done in #12064

fixup - remove parsehtml.lua
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cscheid